Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add types definitions #76

Merged
merged 11 commits into from
Mar 5, 2021
Merged

fix: add types definitions #76

merged 11 commits into from
Mar 5, 2021

Conversation

getlarge
Copy link
Member

The name of the PR says it all.

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mcollina
Copy link
Collaborator

Please add a test for the type using http://npm.im/tsd

types/index.d.ts Outdated Show resolved Hide resolved
@robertsLando robertsLando removed the request for review from mcollina February 19, 2021 07:20
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Show resolved Hide resolved
@robertsLando
Copy link
Member

@mcollina Could you give a look at this?

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@getlarge
Copy link
Member Author

getlarge commented Mar 4, 2021

@robertsLando @mcollina Could you have a look at the changes you requested ?
I will fix the tests tomorrow, for some strange reason they failed because some keys ( packet object ) are not in the order expected.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types are ok, I'm not sure about the solution used to fix tests.

abstract.js Outdated
Comment on lines 8 to 13
function orderedKeys (inputObj) {
return Object.keys(inputObj).sort().reduce((obj, key) => {
obj[key] = inputObj[key]
return obj
}, {})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL that's so tricky 😕 I have all aedes repos with tape to update but with version 5 I think there is definetly something that isn't working... @mcollina thoughts on this? I also tried to open an issue about this on tape: tape-testing/tape#508

They suggested me to use deepLooseEqual but last time I tried seems also thaat wasn't working

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahah i can imagine, you were more brave than me by digging into tape.
I just tried deepLooseEqual and it seems to work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good news, maybe in last year they have fixed something 😆 I was quite sure it wasn't working when I tried, If you check all other aedes repos need this fix...

@robertsLando robertsLando requested a review from mcollina March 5, 2021 07:20
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@robertsLando robertsLando merged commit 778008c into master Mar 5, 2021
@robertsLando robertsLando deleted the declare-types branch March 5, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants